Fix ARCHIVED → TODO transition on Cmd/Ctrl+Enter#17
Fix ARCHIVED → TODO transition on Cmd/Ctrl+Enter#17salmonumbrella wants to merge 3 commits intoRoamJS:mainfrom
Conversation
When pressing Cmd/Ctrl+Enter on an ARCHIVED block, Roam prepends TODO to get TODO+ARCHIVED. This collapses that into just TODO. Also reads block text from Roam's data layer instead of the textarea DOM value, which can lag behind after API updates. Closes RoamJS#4
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a new utility module for normalizing TODO blocks with an ARCHIVED prefix pattern and integrates it into the main handler. The keyboard shortcut logic is refactored to read block text from Roam's data layer rather than directly from the textarea value, enabling consistent detection and handling of ARCHIVED, DONE, and TODO prefixed blocks during block state transitions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@mdroidian broke these up for you! please take a look ❤️ |
|
@mdroidian broke these up for you! please take a look ❤️ (again, again) |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
| // Roam's Cmd/Ctrl+Enter prepends TODO for non-checkbox blocks. | ||
| // If the block starts with ARCHIVED, collapse TODO+ARCHIVED into TODO. | ||
| value = normalizeTodoArchivedPrefix(value); |
There was a problem hiding this comment.
🚩 normalizeTodoArchivedPrefix is unlikely to match in current onTodo call sites
The normalization regex /^(\{\{\[\[TODO\]\]\}\})\s*\{\{\[\[ARCHIVED\]\]\}\}/ requires text to start with {{[[TODO]]}} followed by {{[[ARCHIVED]]}}. Current onTodo callers pass text starting with {{[[DONE]]}} (keydown handler line 378), post-toggle text from checkbox click (line 318), or textarea value from click listener (line 345). None of these would typically produce a {{[[TODO]]}}{{[[ARCHIVED]]}} prefix. The normalization acts as a defensive guard but may never match in practice. Consider whether there's an actual code path that produces this pattern, or if the normalization should be placed elsewhere.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (blockText.startsWith("{{[[ARCHIVED]]}}")) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| e.stopImmediatePropagation(); | ||
| const withoutArchived = blockText.replace( | ||
| /^\{\{\[\[ARCHIVED\]\]\}}\s*/, | ||
| "", | ||
| ); | ||
| const normalized = withoutArchived | ||
| ? `{{[[TODO]]}} ${withoutArchived}` | ||
| : "{{[[TODO]]}}"; | ||
| if (normalized !== blockText) { | ||
| updateBlock({ uid: blockUid, text: normalized }); | ||
| } |
There was a problem hiding this comment.
🚩 ARCHIVED→TODO transition does not trigger onTodo settings
When a block starts with {{[[ARCHIVED]]}}, the keydown handler converts it to {{[[TODO]]}} via updateBlock but does NOT call onTodo(). This means user-configured "On Todo" append text, tag replacements, and other onTodo triggers are skipped for ARCHIVED→TODO transitions. This appears intentional (un-archiving is distinct from creating a new TODO), but it's a behavioral choice worth confirming with the author if "On Todo" settings should also fire when restoring from ARCHIVED state.
Was this helpful? React with 👍 or 👎 to provide feedback.
src/index.ts
Outdated
| } else if (blockText.startsWith("{{[[DONE]]}}")) { | ||
| onTodo(blockUid, blockText); | ||
| } else if (blockText.startsWith("{{[[TODO]]}}")) { | ||
| onDone(blockUid, blockText); | ||
| } |
There was a problem hiding this comment.
🚩 Race condition between onTodo/onDone updateBlock and Roam's Ctrl+Enter toggle
For DONE and TODO cases (lines 377-381), neither e.preventDefault() nor e.stopImmediatePropagation() is called, so Roam will also process Ctrl+Enter and toggle the block prefix. Meanwhile onTodo/onDone may call updateBlock with text that still has the old prefix (e.g., {{[[DONE]]}} modified text). Depending on timing, Roam's toggle and the extension's updateBlock could conflict. This is a pre-existing architectural issue (the old code had the same pattern), not introduced by this PR, but it's worth noting that the callback swap makes modifications more semantically correct — and thus more likely to actually change the text — which could make the race more visible in practice.
Was this helpful? React with 👍 or 👎 to provide feedback.
mdroidian
left a comment
There was a problem hiding this comment.
Thank you for the contribution! Here's a video review:
https://www.loom.com/share/8640bd817d8c4d00bf1a0e0bdddf9bf7
Points
- Observed a regression in ToDoTrigger behavior compared to the previous version.
- Open Devin bug report comments.
- To move forward with a change like this, a full set of unit tests covering each feature/function of this repo will likely be required (ETA unknown), or a Loom video demonstrating the compiled PR running through each feature/function to confirm that no regressions have been introduced.
The single-block handler was reading getTextByBlockUid (pre-toggle data layer) and using inverted routing. This caused a race with Roam's own toggle — onDone/onTodo received text with the wrong prefix, leading to duplicated append text instead of clean toggling. Reverting to textArea.value (which Roam updates synchronously on Ctrl+Enter) with standard routing (DONE→onDone, TODO→onTodo) fixes the regression. The ARCHIVED handler still uses getTextByBlockUid since textarea lags for that specific case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@mdroidian thank you for the thorough video review! Really appreciate you catching that regression. I pushed a fix in 6035965 — the root cause was that the single-block Ctrl+Enter handler was reading The fix reverts the TODO/DONE cases to read Happy to record a Loom walkthrough if you'd like visual confirmation before merging. |
Summary
TODO+ARCHIVED. This normalizes that into justTODO.getTextByBlockUid) instead of the textarea DOM value, which can lag behind after API updates.normalizeTodoArchivedPrefixutility that collapses{{[[TODO]]}} {{[[ARCHIVED]]}}into{{[[TODO]]}}.PR17.for.Droidian.mp4
Test plan
Closes #4
Summary by CodeRabbit
Bug Fixes
New Features